-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: inscriptions settings btn and available balance #6050
base: dev
Are you sure you want to change the base?
Conversation
I initially approved this, but I'm not confident adding |
EDIT: Nvm, this was me and my .env file. |
🚧 Undergoing QA I'll test the following cases manually. Can we also add automated tests for them, even if warranted in separate PR(s) to unblock release of this one? Ideally we'd have automated tests that check on the UI level in addition to the function one, to ensure that updated values appear immediately once their related events occur (i.e. to ensure that caching doesn't incur delays to updating their display as expected by users). 🤔 == cases in which I'm not sure how to test without configuring the dev wallet with special assets Home view – total balance
Home view - available balance
Home view – token balances
Send form view
Swap form view
Select account /
|
This feature, and all the bugs/inaccuracies it introduced are the result of fast-paced work building on top an already old/poorly-implemented feature. We need to rethink how we manage UTXOs, and in turn calculate balances, from ground up. This was already in-flight prior to the sBTC rush. It may well be better to focus on that than first fixing these issues? I think @alexp3y and I should return to this next week, focusing on:
|
Agree here, maybe we can merge this with manually testing and then address the integration tests with the larger service refactor work? |
Note that I intend to complete manual QA of this PR on Monday, and I've already surfaced one case that doesn't seem to work in this PR atm ("Total balance on home view does not update due to pending outbound BTC transfer", in that in my testing, it does update undesirably). Other folks are welcome to contribute to this manual QA in the meantime if they want to go through my list and check things off. cc @DeeList @314159265359879 I'm also in support of the larger code changes (that will help mobile as well presumably). @kyranjamie is your sense that these larger efforts will be needed to get this PR finalized, or might we be able to do them in parallel and patch this PR separately in the meantime? |
Yep, it looks like in code we filter out pending utxos for both total and available BTC balance, so that needs to be changed before this is merged. |
this pr fixes: